Fix bulk_create not setting M2M on FK-related objects#583
Fix bulk_create not setting M2M on FK-related objects#583amureki merged 7 commits intomodel-bakers:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a second post-processing pass in Changes
Sequence DiagramsequenceDiagram
participant User
participant Baker as baker.bulk_create
participant ORM as Django ORM
participant FKObj as FK-related object
User->>Baker: call bulk_create(entries, home__dogs=[dog], _bulk_create=True)
Baker->>Baker: prepare entries, persist FK fields
Baker->>ORM: bulk_create(prepared_entries)
ORM-->>Baker: return created objects
Baker->>Baker: parse single-level `fk__m2m` kwargs
loop per created object
Baker->>FKObj: getattr(entry, fk_field_name) -> fk_obj
Baker->>FKObj: insert through-model rows linking fk_obj and provided m2m objects (bulk_create)
FKObj-->>Baker: through rows persisted
end
Baker-->>User: return created entries
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
e0919e0 to
5574857
Compare
amureki
left a comment
There was a problem hiding this comment.
Hey @benaduo !
Thank you for opening this PR and fighting against linters and typing checks. I understand that this experience could be improved and will try to address things there.
Regarding the change, very well done with handling all the different cases and scenarios here, good guardrails!
I noted a couple of things we could improve before moving forward.
When baker.make() is called with _bulk_create=True and a M2M field is specified via double-underscore FK lookup syntax (e.g. home__dogs=[dog]), the M2M relationship was never applied to the saved FK object. Root cause: during the prepare() phase, baker correctly stores M2M values in the sub-baker's m2m_dict, but _handle_m2m() is gated behind `if _commit:` and therefore never executes in the bulk_create path. Subsequently, _save_related_objs() only saves FK rows and makes no M2M calls. The two existing M2M loops in bulk_create() scan only the top-level model's direct and reverse M2M fields, neither of which covers the fk__m2m kwarg pattern. The net result is that home.dogs.set([dog]) is never called, leaving the M2M relation empty in the database. Fix: after all entries have been bulk-created and FK objects saved, inspect the kwargs dict for double-underscore patterns of the form fk_field__m2m_field. For each pattern where the prefix resolves to a ForeignKey or OneToOneField on the current model and the suffix resolves to a ManyToManyField on the related model, call .set() on the FK object for every created entry. Changes: - model_bakery/baker.py: add FK-nested M2M handling loop at the end of bulk_create(), after the existing reverse-relation M2M loop - tests/generic/models.py: add HomeOwner model with ForeignKey to Home, providing the minimal fixture required to reproduce the reported scenario - tests/test_baker.py: add test_create_through_foreign_key_field to TestCreateM2MWhenBulkCreate, reproducing the exact scenario from the issue (baker.make(HomeOwner, home__dogs=[dog], _quantity=10, _bulk_create=True)) Fixes model-bakers#490 Author: Benjamin Aduo (@benaduo)
When baker.make() is called with _bulk_create=True and a M2M field is specified via double-underscore FK lookup syntax (e.g. home__dogs=[dog]), the M2M relationship was never applied to the saved FK object. Root cause: during the prepare() phase, baker correctly stores M2M values in the sub-baker's m2m_dict, but _handle_m2m() is gated behind `if _commit:` and therefore never executes in the bulk_create path. Subsequently, _save_related_objs() only saves FK rows and makes no M2M calls. The two existing M2M loops in bulk_create() scan only the top-level model's direct and reverse M2M fields, neither of which covers the fk__m2m kwarg pattern. The net result is that home.dogs.set([dog]) is never called, leaving the M2M relation empty in the database. Fix: after all entries have been bulk-created and FK objects saved, inspect the kwargs dict for double-underscore patterns of the form fk_field__m2m_field. For each pattern where the prefix resolves to a ForeignKey or OneToOneField on the current model and the suffix resolves to a ManyToManyField on the related model, call .set() on the FK object for every created entry. Only auto-created through models are supported; fields using a custom through= model are skipped since they require explicit through instance creation with extra field values baker cannot infer. FieldDoesNotExist is caught specifically (not bare Exception) so that unexpected errors still surface rather than being silently swallowed. Changes: - model_bakery/baker.py: import FieldDoesNotExist from django.core.exceptions; add FK-nested M2M handling loop at the end of bulk_create() with specific exception handling and auto-created through model guard - tests/generic/models.py: add HomeOwner model with ForeignKey to Home, providing the minimal fixture required to reproduce the reported scenario - tests/test_baker.py: add test_create_through_foreign_key_field to TestCreateM2MWhenBulkCreate, reproducing the exact scenario from the issue (baker.make(HomeOwner, home__dogs=[dog], _quantity=10, _bulk_create=True)) Fixes model-bakers#490 Author: Benjamin Aduo (@benaduo)
I actually like this. Yes lets do this Co-authored-by: Rust Saiargaliev <hey@amureki.me>
Hmmm... I wanted to be very thorough in my docs ... but yeah I see how this can be harder to read... very nice catch Co-authored-by: Rust Saiargaliev <hey@amureki.me>
This will indeed lead to less db hits... i agree Co-authored-by: Rust Saiargaliev <hey@amureki.me>
bd9e8af to
006da06
Compare
GitHub's commit suggestion feature preserved incorrect indentation from the diff view for the comment block in baker.py (2 spaces instead of 4) and the for loop in test_baker.py (6 spaces instead of 8).
|
Hi @amureki, I've addressed all three suggestions:
I also resolved the merge conflict with main after #582 landed. One thing to flag though: the ruff C901 complexity check is now failing on bulk_create (scoring 19 against the limit of 10). The optimization you suggested pushed it over the threshold. Happy to refactor the function into smaller pieces if that's the preferred approach or follow whatever direction you think is best here. Thanks for the thorough review! |
|
@benaduo thanks for addressing my comments! Again, many thanks for taking the time and headspace for all these contributions, which makes me very happy! 🙏 |
Ah, I see, sorry! |
The function exceeds ruff's complexity limit of 10 (scores 19) after the bulk_create optimization for FK-nested M2M rows was added per reviewer suggestion. Suppressing for now as agreed with maintainer; refactoring into smaller helpers can follow.
|
Ok @amureki, all CI checks are now passing including ruff, black and ty. The PR is ready whenever you are! |
…any-to-one-rel * origin/main: Bump requests from 2.32.5 to 2.33.0 (#590) Fix bulk_create not setting M2M on FK-related objects (#583) Fix remaining ty type errors in baker.py and generators.py (#589) Add regression test for bulk_create with M2M field using explicit related_name (#582) Update uv-build requirement from <0.10.0,>=0.9.26 to >=0.9.26,<0.11.0 (#579) Bump black from 25.12.0 to 26.3.1 (#588) Bump urllib3 from 2.6.2 to 2.6.3 (#587) Bump actions/download-artifact from 7 to 8 (#580) Bump actions/upload-artifact from 6 to 7 (#581) Bump django from 5.2.9 to 5.2.12 (#585) Bump pillow from 12.0.0 to 12.1.1 (#586) Fix type annotations (#584)
Summary
baker.make()with_bulk_create=Truenot applying M2M relationshipswhen the M2M field is on a FK-related object, specified via double-underscore
syntax (e.g.
home__dogs=[dog])HomeOwnertest model and a regression test reproducing the exactscenario from the issue
Motivation
When a M2M value is passed as
fk_field__m2m_field=[...], baker stores thevalue in the sub-baker's
m2m_dictduringprepare(), but_handle_m2m()isgated behind
if _commit:and never executes in the bulk_create path._save_related_objs()only saves FK rows without touching M2M, and the twoexisting M2M loops in
bulk_create()only scan the top-level model's ownfields — neither handles the
fk__m2mkwarg pattern. The result is thathome.dogs.set([dog])is never called and the M2M relation stays empty.Changes
model_bakery/baker.py: importFieldDoesNotExistfromdjango.core.exceptions; add FK-nested M2M handling loop at the end ofbulk_create()— parses kwargs forfk_field__m2m_fieldpatterns, validatesfield types, guards against custom through models, and calls
.set()on theFK object for each bulk-created entry
tests/generic/models.py: addHomeOwnermodel withForeignKeytoHometests/test_baker.py: addtest_create_through_foreign_key_fieldtoTestCreateM2MWhenBulkCreateTesting
pytest tests/test_baker.py::TestCreateM2MWhenBulkCreate -vAuthor
Benjamin Aduo (@benaduo)
Summary by CodeRabbit
New Features
Tests